Conversation
smartie2076
left a comment
smartie2076
left a comment
There was a problem hiding this comment.
Hi @remmyd! I dont have the time to still check the actual code (and if it works), but so far it looks good :)
There are some files that should not have been commited to this branch, namely the .log files and all .pyc files. You need to remove those files from the PR.
I would look into pytests next, try something simple like in B to start with. At the end, you should have a benchmark test that makes sure that your constraint works.
| f"Parameter {EVALUATION_PERSPECTIVE} has to be either {AC_SYSTEM} or {DC_SYSTEM}, but is {case_definitions[case][EVALUATION_PERSPECTIVE]}" | ||
| ) | ||
|
|
||
| if USE_BIDIRECTIONAL_INVERTER_CONSTRAINT not in case_definitions[case]: |
There was a problem hiding this comment.
Looking good. Maybe write a pytest for this function that makes sure that your two outcomes (USE_BIDIRECTIONAL_INVERTER_CONSTRAINT is not and is in settings) are checked.
There was a problem hiding this comment.
yes will look into pytest next
| ) | ||
|
|
||
| if USE_BIDIRECTIONAL_INVERTER_CONSTRAINT not in case_definitions[case]: | ||
| case_definitions[case][USE_BIDIRECTIONAL_INVERTER_CONSTRAINT] = False |
There was a problem hiding this comment.
Especially because of this, I think you need to use json.update(). With the pytest you will figure out if there are bugs in your code, even if the code initially passes.
There was a problem hiding this comment.
What will the json.update() do?
| if case_dict[PCC_FEEDIN_FIXED_CAPACITY] == None: | ||
| pass | ||
| # pointofcoupling_feedin = None | ||
| pointofcoupling_feedin = None |
There was a problem hiding this comment.
Intentional removal of #?
There was a problem hiding this comment.
It is an intentional removal because I need it later on in the code for the constraint_equate_bidirectional_transformer_capacities function (see line 462-471) in case PCC_FEEDIN_FIXED_CAPACITY is None.
| ) | ||
|
|
||
| # ------------Link rectifier and inverter capacities constraint------------# | ||
| if case_dict[USE_BIDIRECTIONAL_INVERTER_CONSTRAINT] != None: |
There was a problem hiding this comment.
can it be None? Isnt your default False?
There was a problem hiding this comment.
Should be false indeed
| CAP_inverter = 0 | ||
| CAP_rectifier = 0 | ||
| if case_dict[name_transformer_in] != None and case_dict[name_transformer_out] != None: | ||
| if case_dict[name_transformer_in] is False and case_dict[name_transformer_out] is False: |
There was a problem hiding this comment.
what does this loop do? Dont forget to use in-line comments (#), and also add docstrings to your functions (typing `r"""`` will probably aid you in writing them)
There was a problem hiding this comment.
alright will add comments for the next PR
There was a problem hiding this comment.
actually I took the idea from one of your comments (#155 (comment)) but I'm not sure in what case case_dict[name_transformer_in] is false and why different capacities need to be added (see code below) since there's just one converter
CAP_inverter += model.InvestmentFlow.invest[bus_transformer_in, transformer_in]
CAP_rectifier += model.InvestmentFlow.invest[bus_transformer_out, transformer_out]
Could you clarify what it represents?
|
If you need inspiration for the pytests, you can find a lot in https://github.com/rl-institut/multi-vector-simulator/, folder |
|
Hello @remmyd! Haven you been able to work on the pytests since my last, short review? If you let me know if this PR is still up-to-date I can do a review next week. |
2 participants
#155 @smartie2076 here's the first draft for the link between inverter rectifier capacity as well as for the transformer station. Let me know what you think when you get the chance